-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: block generator inner transactions #5506
Conversation
Co-authored-by: shiqizng <[email protected]>
Co-authored-by: shiqizng <[email protected]>
for k := range effects { | ||
keys = append(keys, k) | ||
} | ||
sort.Strings(keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now sorting the transactions portion of the local report and adding a sum-total including "effects". For example:
scenario:config.appboxes.small.yml
test_duration_seconds:12
test_duration_actual_seconds:12.022910
transaction_app_boxes_call_total:496
transaction_app_boxes_create_total:106
transaction_app_boxes_optin_total:4260
transaction_effect_inner_tx_total:8520
transaction_effect_payment_sibling_total:4366
transaction_ALL_total:17748
...
Codecov Report
@@ Coverage Diff @@
## master #5506 +/- ##
==========================================
- Coverage 55.83% 55.81% -0.02%
==========================================
Files 446 446
Lines 63223 63223
==========================================
- Hits 35299 35287 -12
- Misses 25561 25572 +11
- Partials 2363 2364 +1 see 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Co-authored-by: shiqizng <[email protected]>
intra := uint64(0) | ||
txGroupsAD := [][]txn.SignedTxnWithAD{} | ||
for intra < minTxnsForBlock { | ||
txGroupAD, numTxns, err := g.generateTxGroup(g.round, intra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the txgroup stuff get pushed into g.evaluateBlock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps lines 319-332 could become:
intra, txGroupsAD := g.generateTxGroups()
Or perhaps lines 319-334 could become:
vBlock, ledgerTxnCount, err := g.generateBlock()
Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generating the tx groups is part of generating the block, so I was thinking you could push 319-340 into g.evaluateBlock
, and rename evaluateBlock
to generateBlock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits, I don't think they need to be addressed in this PR.
Summary
This is a bugfix as inner transactions weren't actually being saved to the database after #5478 . The main problem was that only
ledger.AddBlock()
was used to persist blocks which were missingApplyData
. This function doesn't modify the block which it is adding (it can evaluate a different block for validation purposes though). The fix works by using logic similar to the transaction pool and the recent simulator:eval.StartEvaluator()
withgenerate = true
to instantiate a block evaluatoreval.TransactionGroup()
on each group that the generator provideseval.GenerateBlock()
at the end to create a validated block which contains the evaluatedApplyData
with inner transactionsledger.AddValidatedBlock()
to persist.Additional changes have been hilighted in my comments below.
TODO's Carried Over
The ones that haven't yet been done should be carried over to Part 3: swaps
ApplyData
settings/params asLedger.Eval()
should be in charge of this now and all existing references are empty (generator.go
)effects
map should be slices of a type different fromTxTypeID
(DONE: now they're plain strings)the generator only usesNOPE: this is now being updated from the block's evaluator directly. However, there is still an assertion that the number of expected transactions is what the evaluator provided.effects
map to updatetxnCounter
(orintra
)TxTypeID
with effects is exactly as expected (DONE: see TestEffectsMap )run.go
to split out the various effectsgenerate_apps.go
for application specific generator codegenerator_ledger.go
for ledger specific generator codeintra
(generator.go
)getActualAppCall()
effects
map in the next PR for because of the new teal files and updated namesmake_transactions.go
)case appTxTypeCreate
for the other cases (generateAppCallInternal()
)type appData struct
fieldkind
if still unused after Part 3 (generator_types.go
)generate.go
should become:vBlock, ledgerTxnCount, err := g.generateBlock()
g.evaluateBlock
, and renameevaluateBlock
togenerateBlock
Test Plan
In addition to some new unit tests, I've tested it locally using the same method described on #5478
FAQ
Q: How to calculate the number of inner transactions in the database?
A: